Skip to content

Update .lgd-row classes to use CSS grid #778

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 27, 2025

Conversation

markconroy
Copy link
Member

Closes #666

What does this change?

Rewrites our .lgd-row grid system to use CSS Grid instead of flexbox. This means we will no longer have the annoying issue with the small margins on the left/right of grids that mean things don't line up as nicely as we'd like them to without extra CSS overrides.

How to test

Click around the website and make sure the grid is working, but more importantly marvel at how all the items line up correctly.

How can we measure success?

Waaaaay few overrides for our layout in custom themes.

Images

Screenshot 2025-05-23 at 14 09 48

@msayoung
Copy link
Member

Very happy about this, but there are still some weird alignment issues which I've been meaning to talk to you about.

The culprit is padding-horizontal. I'll add some screenshots

Are you also thinking of rolling this out to the multi-column layouts (in subsites)?

Copy link
Contributor

@ctorgalson ctorgalson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will make a good new default (I typically do more or less this on new projects).

@ctorgalson
Copy link
Contributor

ctorgalson commented May 26, 2025

The culprit is padding-horizontal. I'll add some screenshots

Yes, definitely. The basic issue is that it's used everywhere--on page-level layouts, regions, and components. It means that sometimes .padding-horizontal is applied to an element, its parent and its grand-parent element.

It's hard to think about how to roll out fixes for it to existing sites without breaking all the overrides we incorporated when building them in the first place. On new sites, it's possible to use some slightly hairy css to keep a lid on the problem, something like the code below, but it's design-dependent, so the selectors in the :where() clauses change from site to site:

Site-specific .padding-horizontal rationalization
/**
 * LGD's .padding-horizontal can be unhelpful, especially when nested.
 *
 * This file selectively overrides .padding-horizontal by region and content
 * type, essentially by removing padding-inline from the element with the class
 * and adding it to the parent element, making exceptions for the children of
 * certain grid-cells.
 *
 * The main selector does the following:
 *
 * - :where():has(): targets specific regions and specific 'full' content types
 *     that have a child element matching .padding-horizontal,
 * - .lgd-row:not() &: (content types only) targets elements from the
 *     :where():has() that are NOT children of specific grid-cells, and ADDS
 *     padding-inline to them,
 * - & > .padding-horizontal: targets child elements from the :where:has() that
 *     have the .padding-horizontal class and REMOVES padding-inline from them.
 */

/* Regions are relatively simple */
:where(
  /* Add regions where .padding-horizontal isn't helping. */
  .region-breadcrumb,
  .region-content-bottom,
  .region-messages,
  .region-tabs,
  .region-content-top,
):has(> .padding-horizontal) {
  padding-inline: var(--spacing-mobile-gutter);

  & > .padding-horizontal {
    padding-inline: 0;
  }
}

/* Content types are slightly trickier */
:where(
  /* Add node content elements here where .padding-horizontal isn't helping. */
  .lgd-maintenance,
  .lgd-status,
  .localgov-alert-banner,
  .localgov-services-page.node--view-mode-full,
  .localgov-subsites-overview.node--view-mode-full,
  .node--type-localgov-guides-overview.node--view-mode-full,
  .node--type-localgov-guides-page.node--view-mode-full,
  .node--type-localgov-news-article.node--view-mode-full,
  .localgov-subsites-overview.node--view-mode-full,
  .localgov-step-by-step-overview.node--view-mode-full,
  .localgov-step-by-step-page.node--view-mode-full,
  .lgd-page-section--full-width-contained-content,
):has(> .padding-horizontal) {
  /* Add padding-inline to regions/node-content on non-sidebar pages. */
  .main:not(:has(.sidebar)) & {
    padding-inline: var(--spacing-mobile-gutter);
  }

  & > .padding-horizontal {
    padding-inline: 0;
  }
}

/**
 * Children of .main--this doesn't include regions--get styled to match.
 *
 * NOTE: some components handle this in their own stylesheets as they require
 *   further customization.
 */
.main > .lgd-region--content:first-child,
.main > .lgd-region--content-top + .lgd-region--content,
.main > .lgd-container {
  margin-block-start: var(--vertical-rhythm-spacing);
}

.main > .lgd-container {
  box-sizing: content-box;
  margin-inline: auto;
  max-width: var(--width-extra-large);
  padding-inline: var(--spacing-mobile-gutter);
}

/* Remove padding-inline from immediate children of grid cells generally. */
.lgd-row
  > :where(
    .lgd-row__one-quarter,
    .lgd-row__three-quarters,
    .lgd-row__one-third,
    .lgd-row__two-thirds,
    .lgd-row__one-half,

  )
  > .padding-horizontal {
  padding-inline: 0;
}

@media screen and (min-width: 64rem) {
  .main > .lgd-region--content:first-child,
  .main > .lgd-region--content-top + .lgd-region--content,
  .main > .lgd-container {
    margin-block-start: calc(2 * var(--vertical-rhythm-spacing));
  }
}

On a recent non-LGD site, I introduced a new class .section with a lot of BEM style variants like:

  • .section--bleed (no padding-inline)
  • .section--compact (no/reduced margin-block)
  • .section--contained (max-width for direct children)
  • etc. (there were three or four others)

As long as regions, components used this basic structure, everything worked perfectly well, and it was possible to mix and match the .section--* classes to combine their effects:

<div class="section section--contained">
  <div class="section__inner">
    <!-- Content -->
  </div>
</div>

It would work for LGD too, though without a major version increase, all we could really do would be to ship a stylesheet that was scoped by a non-default style, or not added to the theme's libraries file--we'd still have to remove/override the existing .padding-horizontal classes.

@markconroy
Copy link
Member Author

though without a major version increase

I'm definitely not against a major version increase.

@finnlewis
Copy link
Member

Discussing in Merge Tuesday, we're happy with this as a minor version release, not major.

@finnlewis finnlewis merged commit ffa1c45 into 1.x May 27, 2025
6 checks passed
finnlewis added a commit that referenced this pull request May 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite our grid system to use CSS Grid
4 participants